Skip to content

change attrs to dataclasses - #521

Merged
inducer merged 14 commits into
inducer:mainfrom
illinois-ceesd:attrs-dc
Nov 19, 2024
Merged

change attrs to dataclasses#521
inducer merged 14 commits into
inducer:mainfrom
illinois-ceesd:attrs-dc

Conversation

@matthiasdiener

@matthiasdiener matthiasdiener commented Jul 17, 2024

Copy link
Copy Markdown
Contributor

Closes #370.

TODOs:

  • restore validators
  • (cached) hashing

Please squash

@matthiasdiener matthiasdiener self-assigned this Jul 17, 2024
@matthiasdiener
matthiasdiener marked this pull request as ready for review July 17, 2024 22:00
@matthiasdiener
matthiasdiener marked this pull request as draft July 17, 2024 22:00
@matthiasdiener
matthiasdiener requested a review from inducer July 17, 2024 22:49
@matthiasdiener

Copy link
Copy Markdown
Contributor Author

This is ready for a first look.

@inducer inducer left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this started. Two points to work on below.

Comment thread pytato/array.py Outdated
Comment on lines -1128 to -1134
attrs.field(validator=attrs.validators.instance_of(immutabledict))
index_to_access_descr: Mapping[str, EinsumAxisDescriptor] = \
attrs.field(validator=attrs.validators.instance_of(immutabledict))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation can happen as part of __post_init__.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2da059ea

Comment thread pytato/array.py Outdated


@attrs.frozen(eq=False, repr=False, hash=True, cache_hash=True)
@dataclasses.dataclass(frozen=True, eq=False, repr=False, unsafe_hash=True)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where applicable, all array-like dataclasses should use the same, unparametrized decorator.

Example: https://github.com/inducer/pytools/blob/5afdb2d7ea8b40b9ea226393a0e94a61860d83e3/pytools/tag.py#L128-L133

That same decorator should also generate code for a cached hash function. (Maybe this can be factored out into a separate function?) Example: https://github.com/inducer/pymbolic/pull/125/files#diff-768f1abbde39f5f7efff7c209a95315a87e28e0ba4f42e7a468903203aba8ffcR718

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f78cdbf, 4590326.

@inducer

inducer commented Jul 18, 2024

Copy link
Copy Markdown
Owner

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

Comment thread pytato/array.py
Comment on lines -876 to +929
_data: Mapping[str, Array] = attrs.field(
validator=attrs.validators.instance_of(immutabledict))
_data: Mapping[str, Array]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't get this particular validator to work; why should _data be an immutable dict here?

@matthiasdiener
matthiasdiener marked this pull request as ready for review October 16, 2024 20:02
@matthiasdiener

Copy link
Copy Markdown
Contributor Author

This is ready for review.

@inducer
inducer enabled auto-merge (squash) November 19, 2024 22:23
@inducer

inducer commented Nov 19, 2024

Copy link
Copy Markdown
Owner

Thanks!

Comment thread pytato/loopy.py Outdated
@inducer
inducer merged commit 0f643da into inducer:main Nov 19, 2024
@matthiasdiener
matthiasdiener deleted the attrs-dc branch November 20, 2024 18:38
nkoskelo pushed a commit to nkoskelo/pytato that referenced this pull request Nov 25, 2024
* change attrs to dataclasses

* post-merge fix

* more fixes

* switch to array_dataclass

* use _augment_array_dataclass

* restore attribute validation

* mypy?

* Disable pylint invalid-field-call

---------

Co-authored-by: Andreas Kloeckner <inform@tiker.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Post-review of transition to attrs

2 participants